Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: make commit guidelines easier to reference #11732

Closed
wants to merge 1 commit into from

Conversation

bf4
Copy link
Contributor

@bf4 bf4 commented Mar 7, 2017

  • Can now link to [Commit Guidelines](CONTRIBUTING.md#commit-guidelines) from pull requests
  • Breaks up commit requirements and recommendations

Refs: #11723 (comment)

Checklist
Affected core subsystem(s)

None

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Mar 7, 2017
@bf4 bf4 changed the title <!-- CONTRIBUTING: make commit guidelines easier to reference Mar 7, 2017
@bf4
Copy link
Contributor Author

bf4 commented Mar 7, 2017

(first attempt to use 'hub' to use the template.. failed)

Trott
Trott previously requested changes Mar 7, 2017
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to do this! I definitely have some concerns about some of the links, but I do like the added header and the re-organization of the info.

CONTRIBUTING.md Outdated
@@ -12,6 +12,9 @@ When opening new issues or commenting on existing issues on this repository
please make sure discussions are related to concrete technical issues with the
Node.js software.

Go to https://github.com/nodejs/node/issues/new and click the 'New Issue' button
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm definitely -1 on including the bare URL. I'm not sure any of this paragraph is needed at all. Do we have any reason to believe that people don't know how to open issues on GitHub and are looking for the information in the CONTRIBUTING.md? Most people only see the CONTRIBUTING.md when they are already opening a PR or issue because that's when GitHub shows a link to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In retrospect, I agree. :)

CONTRIBUTING.md Outdated
@@ -12,6 +12,9 @@ When opening new issues or commenting on existing issues on this repository
please make sure discussions are related to concrete technical issues with the
Node.js software.

Go to https://github.com/nodejs/node/issues/new and click the 'New Issue' button
and [fill out the form](.github/ISSUE_TEMPLATE.md).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This link is misleading. The text implies that you will be taken to a form. Instead, it will take you to a markdown file. I think this entire paragraph should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call.

CONTRIBUTING.md Outdated
@@ -203,7 +208,7 @@ $ git push origin my-branch
```

Go to https://github.com/yourusername/node and select your branch.
Click the 'Pull Request' button and fill out the form.
Click the 'Pull Request' button and [fill out the form](.github/PULL_REQUEST_TEMPLATE.md).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This link is misleading. The text implies that you will be taken to a form. Instead, it will take you to a markdown file. I think this entire paragraph should be removed.

@@ -2,7 +2,7 @@

// This value is used to prevent the nextTickQueue from becoming too
// large and cause the process to run out of memory. When this value
// is reached the nextTimeQueue array will be shortend (see tickDone
// is reached the nextTimeQueue array will be shortened (see tickDone
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bf4 could you raise this in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Didn't know which would ultimately be more overhead

test/README.md Outdated
@@ -5,7 +5,7 @@ This folder contains code and data used to test the Node.js implementation.
For a detailed guide on how to write tests in this
directory, see [the guide on writing tests](../doc/guides/writing-tests.md).

On how to run tests in this direcotry, see
On how to run tests in this directory, see
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated change.

CONTRIBUTING.md Outdated
lowercase with the exception of proper nouns, acronyms, and the ones that
refer to code, like function/variable names. The description should
be prefixed with the name of the changed subsystem and start with an
#### Commit Message Guidelines
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 to adding a header for the formatting guidelines.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also 👍 to the re-organization and consolidation.

@mscdex mscdex added the meta Issues and PRs related to the general management of the project. label Mar 7, 2017
Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nits, but a big +1 on the new layout

CONTRIBUTING.md Outdated
1. The first line (the header) should:
- contain a short description of the change.
- be 50 characters or less.
- be entirely in lowercase with the exception of proper nouns, acronyms, and the ones that
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the ones -> words

CONTRIBUTING.md Outdated
#### Commit Message Guidelines

1. The first line (the header) should:
- contain a short description of the change.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need full stops at the end of these, it's all one run-on sentence after the colon (I guess you could use commas if you wanted to, but I don't think it's helpful).

@@ -2,7 +2,7 @@

// This value is used to prevent the nextTickQueue from becoming too
// large and cause the process to run out of memory. When this value
// is reached the nextTimeQueue array will be shortend (see tickDone
// is reached the nextTimeQueue array will be shortened (see tickDone
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bf4 could you raise this in a separate PR.

CONTRIBUTING.md Outdated
be prefixed with the name of the changed subsystem and start with an
#### Commit Message Guidelines

1. The first line (the header) should:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the header -> the subject line (I've never seen it called the header)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's referred to as the header below. I was just conserving the language so that later references to 'the header' would be understandable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, but there don't seem to be any more references to "the header" as of this PR, so I think we should be fine using subject line instead?

CONTRIBUTING.md Outdated
- be 50 characters or less.
- be entirely in lowercase with the exception of proper nouns, acronyms, and the ones that
refer to code, like function/variable names.
- be prefixed with the name of the changed subsystem and start with an
imperative verb. Example: "net: add localAddress and localPort
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example -> Examples

CONTRIBUTING.md Outdated
to Socket", "src: fix typos in node_lttng_provider.h"
- be meaningful; it is what other people see when they
run `git shortlog` or `git log --oneline`.<br>
Check the output of `git log --oneline files_that_you_changed` to find out
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe files_that_you_changed -> files/you/changed to emphasize that it's a path

CONTRIBUTING.md Outdated
2. Keep the second line blank.
3. Wrap all other lines at 72 columns.
3. Wrap all other lines in the body at 72 columns.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the body seems unnecessary here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it for that same reason as #11732 (comment), where it's later referred to as the body.

Copy link
Member

@gibfahn gibfahn Mar 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, then maybe just say Wrap the body at 72 columns, otherwise it's unclear whether there are other lines not in the body that might not need to be wrapped.

@bf4 bf4 force-pushed the clarify_commit_guidelines branch from 85ce194 to 80fa742 Compare March 7, 2017 23:42
@bf4
Copy link
Contributor Author

bf4 commented Mar 7, 2017

@Trott @gibfahn I removed the typo commit entirely, and added a second commit that contains the changes per feedback.

Fixes: https://github.com/nodejs/node/issues/1337
Refs: http://eslint.org/docs/rules/space-in-parens.html
Refs: https://github.com/nodejs/node/pull/3615
```
Copy link
Contributor Author

@bf4 bf4 Mar 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ Maybe also check your commit for spelling and clarity ? :)

@Trott Trott dismissed their stale review March 8, 2017 05:42

Requested changes have been made.

imperative verb. Examples: "net: add localAddress and localPort
to Socket", "src: fix typos in node_lttng_provider.h"
- be meaningful; it is what other people see when they
run `git shortlog` or `git log --oneline`.<br>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the html <br> is a typo here?

Copy link
Contributor Author

@bf4 bf4 Mar 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 I thought the line break improved readability

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can put a blank line in the file to get a break

Copy link
Contributor Author

@bf4 bf4 Mar 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sam-github I made the 80 char change, but left the <br>. Please review my screenshots and let me know which you prefer.

With brwithout brwith blank line

now

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't tell the difference, they all look fine

@gibfahn
Copy link
Member

gibfahn commented Mar 12, 2017

@bf4 can you change .github/PULL_REQUEST_TEMPLATE.md as #11792 does?

cc/ @sam-github

CONTRIBUTING.md Outdated
be prefixed with the name of the changed subsystem and start with an
imperative verb. Example: "net: add localAddress and localPort
to Socket"
#### Commit Message Guidelines
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if you think this should make a distinction between the pr title and description vs. the various commits that might make up a PR, including ones from feedback.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think any of this applies to the PR title and description, you can do what you want with them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth mentioning that these rules apply to the commit message not to the PR title though, as often people aren't aware of the difference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, well, I kind of read it as meaning the pr title and description, assuming most prs are squashed. I haven't been following that in my commits since the first one....

@bf4 bf4 force-pushed the clarify_commit_guidelines branch 3 times, most recently from a2a20e5 to 49451a9 Compare March 12, 2017 19:23
@bf4
Copy link
Contributor Author

bf4 commented Mar 12, 2017

@gibfahn Added link from PR Template, cc @sam-github

@@ -13,7 +13,7 @@ Contributors guide: https://github.com/nodejs/node/blob/master/CONTRIBUTING.md
- [ ] `make -j4 test` (UNIX), or `vcbuild test` (Windows) passes
- [ ] tests and/or benchmarks are included
- [ ] documentation is changed or added
- [ ] commit message follows commit guidelines
- [ ] commit message follows [commit guidelines](../CONTRIBUTING.md#commit-message-guidelines)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirming the ../ will link correctly when the PR is created

Copy link
Member

@gibfahn gibfahn Mar 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirming the ../ will link correctly when the PR is created

It won't link properly when you raise a PR, as the PR comes under https://github.com/nodejs/node/compare. You can try it with a draft PR (or by editing this PR's title, but none of the relative links work:

Had a similar issue here.

EDIT: Looks like you already fixed it!

@bf4 bf4 force-pushed the clarify_commit_guidelines branch from 49451a9 to 13d97b8 Compare March 12, 2017 19:29
@bf4 bf4 changed the title CONTRIBUTING: make commit guidelines easier to reference doc: make commit guidelines easier to reference Mar 12, 2017
@gibfahn
Copy link
Member

gibfahn commented Mar 17, 2017

@bf4 #11792 has been landed, so this'll need a rebase.

@sam-github
Copy link
Contributor

@bf4 nice layout improvements here, can you rebase and re-push?

@bf4 bf4 force-pushed the clarify_commit_guidelines branch from 13d97b8 to adbf17f Compare March 24, 2017 01:32
@bf4
Copy link
Contributor Author

bf4 commented Mar 24, 2017

@gibfahn @sam-github Rebased off of master; Added commit to accommodate #11792

Let me know if you'd like me to squash down to one commit.

CONTRIBUTING.md Outdated
1. The first line should:
- contain a short description of the change
- be 50 characters or less
- be entirely in lowercase with the exception of proper nouns, acronyms, and the words that
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrap to 80 colums

imperative verb. Examples: "net: add localAddress and localPort
to Socket", "src: fix typos in node_lttng_provider.h"
- be meaningful; it is what other people see when they
run `git shortlog` or `git log --oneline`.<br>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can put a blank line in the file to get a break

@sam-github
Copy link
Contributor

@bf4 squashing to one commit would be helpful, thanks

@bf4 bf4 force-pushed the clarify_commit_guidelines branch from adbf17f to 84200a4 Compare March 30, 2017 22:03
CONTRIBUTING.md Outdated
- be meaningful; it is what other people see when they
run `git shortlog` or `git log --oneline`.

Check the output of `git log --oneline files/you/changed` to find out
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't look right without the <br>

screen shot 2017-03-30 at 5 04 06 pm

@bf4 bf4 force-pushed the clarify_commit_guidelines branch from 84200a4 to 17f31e0 Compare March 30, 2017 22:04
CONTRIBUTING.md Outdated
to Socket", "src: fix typos in node_lttng_provider.h"
- be meaningful; it is what other people see when they
run `git shortlog` or `git log --oneline`.
Check the output of `git log --oneline files/you/changed` to find out
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the br and just a line break, doesn't do the <br> either

screen shot 2017-03-30 at 5 10 29 pm

- Can now link to 'Commit Guidelines' from pull requests
- Breaks up commit requirements and recommendations
@bf4 bf4 force-pushed the clarify_commit_guidelines branch from 17f31e0 to 501d159 Compare March 30, 2017 22:11
Copy link
Contributor Author

@bf4 bf4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sam-github I made the 80 char change, but left the <br>. Please review my screenshots and let me know which you prefer. https://github.com/nodejs/node/pull/11732/files#r109050698

I also squashed the commits and rebased against master.

Let me know what else I can do. Sorry for the delay in getting back to you.

@benjamingr
Copy link
Member

@sam-github are you content with the latest changes?

@gibfahn
Copy link
Member

gibfahn commented Apr 18, 2017

@Trott @fhinkel LGTY?

@Trott
Copy link
Member

Trott commented Apr 19, 2017

@gibfahn No objections from me.

@bf4
Copy link
Contributor Author

bf4 commented Apr 21, 2017

Anything else for me to do?

@gibfahn
Copy link
Member

gibfahn commented Apr 22, 2017

Landed in bd97e48

@gibfahn gibfahn closed this Apr 22, 2017
gibfahn pushed a commit that referenced this pull request Apr 22, 2017
- Can now link to 'Commit Guidelines' from pull requests
- Breaks up commit requirements and recommendations

PR-URL: #11732
Refs: #11723 (comment)
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
@bf4 bf4 deleted the clarify_commit_guidelines branch April 24, 2017 02:20
evanlucas pushed a commit that referenced this pull request Apr 25, 2017
- Can now link to 'Commit Guidelines' from pull requests
- Breaks up commit requirements and recommendations

PR-URL: #11732
Refs: #11723 (comment)
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
@evanlucas evanlucas mentioned this pull request May 1, 2017
evanlucas pushed a commit that referenced this pull request May 1, 2017
- Can now link to 'Commit Guidelines' from pull requests
- Breaks up commit requirements and recommendations

PR-URL: #11732
Refs: #11723 (comment)
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
- Can now link to 'Commit Guidelines' from pull requests
- Breaks up commit requirements and recommendations

PR-URL: #11732
Refs: #11723 (comment)
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
gibfahn pushed a commit that referenced this pull request May 16, 2017
- Can now link to 'Commit Guidelines' from pull requests
- Breaks up commit requirements and recommendations

PR-URL: #11732
Refs: #11723 (comment)
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 18, 2017
- Can now link to 'Commit Guidelines' from pull requests
- Breaks up commit requirements and recommendations

PR-URL: #11732
Refs: #11723 (comment)
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 23, 2017
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
- Can now link to 'Commit Guidelines' from pull requests
- Breaks up commit requirements and recommendations

PR-URL: nodejs/node#11732
Refs: nodejs/node#11723 (comment)
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants